Apply the visibility answer the Agents screen already recorded - #680
Conversation
The field was absent from the wire models, so the flow asked who may read future sessions and step 3 overwrote the answer seconds later. It rides the Agents decision, so it is read off the same answer and validated against the CLI closed set rather than forwarded: the value lands in profile config and is stamped on every session after.
Falling through to the prompt does not leave the profile alone: its cursor starts on org_public, so a Return on a re-run widens an existing private on a question already answered. Whether the step settled separates the two nulls, and DecideVisibility is extracted because step 3 interactive branches had no coverage at all.
PR Summary by QodoApply browser visibility choice during CLI setup
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Its claim is that the attempt gave up rather than waiting the 30s hold, but its bound was the hook 5s budget itself, so a loaded runner overshot it by 240ms with the spool correct. The two sibling tests for the same claim use 15s and 10s; this brings the outlier into line, still well under the hold.
| /// anything about the profile — see <c>SetupCommand.DecideVisibility</c>, which separates them by | ||
| /// whether the step settled, because the second has told the machine nothing.</para> | ||
| /// | ||
| /// <para><b>It can name a stop this tenant does not currently offer</b>, because the server's fold |
There was a problem hiding this comment.
[P3] Keep the wire contract aligned with closed-set validation
This paragraph says a future/unsupported visibility is still written, but FirstRunFlowOutcomes.Visibility deliberately rejects anything outside AppConfig.ValidVisibilities and converts it to null, after which setup preserves the current profile. That closed-set boundary is also explicitly tested. Please document the actual drop-and-preserve behavior; the current contract states the opposite and could lead a later caller to bypass the validation.
There was a problem hiding this comment.
Agreed, and it was the wrong way round rather than merely vague - fixed in 2c38a35. The paragraph now states drop-and-preserve and names AppConfig.ValidVisibilities as the boundary, so the doc points at the validation instead of inviting a bypass of it.
The behaviour was already what you describe and is pinned by tests (an unknown stop and an empty string both degrade to null); only the doc disagreed. I checked the spec and CHANGES for the same claim - both phrase it as the counterfactual argument for validating and then say a dropped value degrades to null, so they were already right.
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
Static review found one actionable documentation-contract inconsistency: unsupported future visibility values are documented as writable even though the implementation deliberately drops them at the closed-set boundary. I left the concrete correction inline. Per request, I did not build or run tests.
The paragraph said an unrecognised stop is still written, which is the opposite of what the boundary does: it is mapped through the closed set and degrades to null. A caller reading the doc could reasonably have bypassed the validation.
|
NO FINDINGS |
2 participants
AI-2282
What & why
The Agents step asks who may read the sessions this machine records from now on, records it on
FirstRunAgentsDecidedEvent, and serves it on the poll asdefault_visibility. No CLI read it: the field was absent fromCapacitor.Cli.Core/FirstRun's wire models, so it was dropped at deserialisation, andkcap setup's step 3 then prompted unconditionally and wrote its own answer over it — the one place in the flow that asked a question and discarded the answer.It now rides the Agents answer, since it comes from the same decision and the same timestamp, and step 3 spends it instead of asking again.
Where to look
org_public, so a Return on a re-run would widen an existingprivateon a question the user had already answered. An answered-but-unset screen therefore re-writes what the profile already holds — the lane's contract for a null answer, and a no-op downstream.SetupCommand.DecideVisibilityis the single place that decides, extracted becauseHandleSetupAsync's interactive branches have no coverage at all (everyHandleAsync_*test drives--no-prompt, which never reaches the browser leg).AppConfig.ValidVisibilities, never forwarded. This is the one field on the lane that persists locally: it lands in profile config and is stamped on every session afterwards, so a stop a newer server invented would be written to a file this build owns and read back by something whose meaning for it cannot be checked here.--default-visibility. That flag is read only under--no-prompt, and the browser leg is skipped there entirely, so the two can never both be present. Interactively the flag is ignored today, before and after this change; making it live is a separate behaviour change.VisibilityLabelso the prompt's converter and the browser-answer line cannot describe the same stop differently.Verification
Core 2425, Cli 3718, App 1161, Integration 233 — all pass, bar two unrelated timing tests that pass in isolation (
Callback_fault_losing_to_dispose_is_logged_once_and_run_settles_detached, local IPC; andCodex_StdoutHandshake_UnaffectedByALargeUnreachableSpoolBacklog).dotnet publish -c Releaseclean, no IL2026/IL3050.Five mutations, each killed. On the boundary: ignoring the validation, and never reading the field — both needed reshaping to compile, since dropping the reference trips IDE0005/IDE0051 as errors, which is analyzer protection rather than coverage. On the rule: an answered-but-unset screen falling through to the prompt, an unsettled screen treated as answered, and the kept branch inventing a fallback instead of carrying the profile's value.
The field is round-tripped through the source-generated JSON context against WireMock, present and absent — nothing else covered that, since every other test builds
FirstRunFlowResponsedirectly, so a naming or AOT-binding slip would have left the profile untouched for ever with the suite green.